Skip to content

Conversation

benma
Copy link
Contributor

@benma benma commented Oct 3, 2025

Receive (verify address) and spend.

Receive (verify address) and spend.
@benma benma requested a review from bznein October 3, 2025 09:37
@benma
Copy link
Contributor Author

benma commented Oct 9, 2025

ping @bznein

Copy link
Collaborator

@bznein bznein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with just a comment about style :)

Comment on lines 1097 to 1129
keypathAccount := []uint32{
48 + hardenedKeyStart,
0 + hardenedKeyStart,
0 + hardenedKeyStart,
2 + hardenedKeyStart,
}

changeKeypath := append(append([]uint32{}, keypathAccount...), 1, 0)
inputKeypath := append(append([]uint32{}, keypathAccount...), 0, 0)

ourXPub, err := device.BTCXPub(coin, keypathAccount, messages.BTCPubRequest_XPUB, false)
require.NoError(t, err)

xpubs := []string{
ourXPub,
"xpub6Esa6esRHkbuXtbdDKqu3uWjQ1GpK39WW2hxbUAN4L3TxrwDyghEwBtUYZ8uK8LZh3tJ3pjWEpxng9tjfo7RT9BaZKV2T3EPvmZ6N1LgSdj",
"xpub6FJ6FAAFUzuWQAKyT98Ngs6UwsoPfPCdmepqX2aLLPT54M85ARsWzPciFd49foStMwhWgfiHP6PnMgPrWLrBJpUHgqw8vZPd5ov8uSfW2vo",
}
ourXPubIndex := uint32(0)
threshold := 1

_, inputPkScript := multisigP2WSH(threshold, xpubs, false, 0)

scriptConfig, err := NewBTCScriptConfigMultisig(uint32(threshold), xpubs, ourXPubIndex)
require.NoError(t, err)

// The multisig account has to be registered if not already.
registered, err := device.BTCIsScriptConfigRegistered(coin, scriptConfig, keypathAccount)
require.NoError(t, err)
require.False(t, registered)

err = device.BTCRegisterScriptConfig(coin, scriptConfig, keypathAccount, "My multisig account")
require.NoError(t, err)
Copy link
Collaborator

@bznein bznein Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm not mistaken, most of this is identical to the test before (and I think also the one in the other file?). I think it's worth to extract it to a helper function to avoid duplicating code and make it easier to maintain.

I think the only differences are


		changeKeypath := append(append([]uint32{}, keypathAccount...), 1, 0)
		inputKeypath := append(append([]uint32{}, keypathAccount...), 0, 0)

and

		_, inputPkScript := multisigP2WSH(threshold, xpubs, false, 0)

Which are only used later and could be defined closer to where they are needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which one exactly? The one before it tests BTCAddress, not BTCSign. The other one is BTCSignPSBT, and creating the PSBT is similar but still quite different.

Copy link
Collaborator

@bznein bznein Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that lines 1094 to 1129 are identical to lines 996 to 1028, with the exception of the creation of this vars:

		changeKeypath := append(append([]uint32{}, keypathAccount...), 1, 0)

		inputKeypath := append(append([]uint32{}, keypathAccount...), 0, 0)


		_, inputPkScript := multisigP2WSH(threshold, xpubs, false, 0)

And on the other one:

		receiveKeypath := append(append([]uint32{}, keypathAccount...), 0, 0)

Which are not used within those blocks and can be left out.

And the same for the other file, I believe all of this code is shared between all three tests:

		t.Helper()

		coin := messages.BTCCoin_BTC

		keypathAccount := []uint32{
			48 + hardenedKeyStart,
			0 + hardenedKeyStart,
			0 + hardenedKeyStart,
			2 + hardenedKeyStart,
		}


	ourXPub, err := device.BTCXPub(coin, keypathAccount, messages.BTCPubRequest_XPUB, false)
		require.NoError(t, err)

		xpubs := []string{
			ourXPub,
			"xpub6Esa6esRHkbuXtbdDKqu3uWjQ1GpK39WW2hxbUAN4L3TxrwDyghEwBtUYZ8uK8LZh3tJ3pjWEpxng9tjfo7RT9BaZKV2T3EPvmZ6N1LgSdj",
			"xpub6FJ6FAAFUzuWQAKyT98Ngs6UwsoPfPCdmepqX2aLLPT54M85ARsWzPciFd49foStMwhWgfiHP6PnMgPrWLrBJpUHgqw8vZPd5ov8uSfW2vo",
		}
		ourXPubIndex := uint32(0)
		threshold := 1

		scriptConfig, err := NewBTCScriptConfigMultisig(uint32(threshold), xpubs, ourXPubIndex)
		require.NoError(t, err)

And can be extracted

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added commit, created by deepseek 🤓

Copy link
Collaborator

@bznein bznein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks for the changes :)

@benma benma merged commit f927d5d into BitBoxSwiss:master Oct 13, 2025
3 checks passed
@benma benma deleted the multisig-tests branch October 13, 2025 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants